Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TS Migration] Style fixes in Text.tsx #30497

Conversation

BartoszGrajdek
Copy link
Contributor

@BartoszGrajdek BartoszGrajdek commented Oct 27, 2023

Details

After migrating a few components to TS it turns out that styles for Text component were typed incorrectly. This PR fixes these problems.

Fixed Issues

N/A

Tests

  • Send a few messages, use decorations like *text* to check if different types of styles are working
  • Send money request
  • Add a task
  • Verify that no errors appear in the JS console

Offline tests

QA Steps

Check if text is styled correctly i.e.:

  1. Send a few messages, use text decorations like *text* or ~text~
  2. Send money requests (IOU)
  3. Add a task
  4. Overall check main functionalities, and if anything looks different than it used to
  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
web.mov
Mobile Web - Chrome
chrome.mov
Mobile Web - Safari
safari.mov
Desktop
desktop.mov
iOS
ios.mov
Android
android.mov

Copy link
Contributor

@blazejkustra blazejkustra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, fix tests and test it extensively 😄

@BartoszGrajdek BartoszGrajdek marked this pull request as ready for review October 27, 2023 13:15
@BartoszGrajdek BartoszGrajdek requested a review from a team as a code owner October 27, 2023 13:15
@melvin-bot melvin-bot bot requested review from chiragsalian and removed request for a team October 27, 2023 13:15
@melvin-bot
Copy link

melvin-bot bot commented Oct 27, 2023

@chiragsalian Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

Copy link
Contributor

@chiragsalian chiragsalian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chiragsalian chiragsalian merged commit 28a1b81 into Expensify:main Oct 27, 2023
15 of 16 checks passed
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@github-actions github-actions bot added the DeployBlockerCash This issue or pull request should block deployment label Oct 27, 2023
@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

Name Duration
App start TTI 1190.125 ms → 1246.915 ms (+56.790 ms, +4.8%)
App start runJsBundle 821.877 ms → 876.907 ms (+55.030 ms, +6.7%) 🔴
Show details
Name Duration
App start TTI Baseline
Mean: 1190.125 ms
Stdev: 37.625 ms (3.2%)
Runs: 1121.3064019999001 1121.605951000005 1121.8083780000452 1146.1254769999068 1146.6566389999352 1148.8869119999 1149.9536339999177 1150.3602150001097 1152.8860830001067 1153.0972249999177 1154.7894210000522 1155.823492999887 1161.6069789999165 1163.4406039998867 1163.7537680000532 1163.9681090000086 1166.936771000037 1166.9395840000361 1167.7746659999248 1171.0692000000272 1171.3512099999934 1171.9371319999918 1172.7379630000796 1174.8199390000664 1175.3087740000337 1182.2459090000484 1184.5134099998977 1188.489915000042 1189.4692349999677 1190.6424370000605 1191.2963469999377 1193.3783559999429 1194.3873729999177 1194.7321079999674 1195.7600080000702 1196.9734489999246 1197.5611590000335 1197.9925450000446 1202.2743949999567 1204.4766460000537 1204.6965779999737 1205.7725919999648 1207.5722199999727 1210.412142999936 1210.4506600000896 1221.453140999889 1221.900876000058 1228.960223000031 1237.2428510000464 1245.617401999887 1245.7551229998935 1250.6761469999328 1255.8565340000205 1263.7303069999907 1266.7423239999916 1268.452609000029 1272.6980349998921

Current
Mean: 1246.915 ms
Stdev: 28.541 ms (2.3%)
Runs: 1187.8908549998887 1189.5424550001044 1197.4075750000775 1197.7596579999663 1198.3561849999242 1208.3738539998885 1208.6751580000855 1209.4490149999037 1212.502439999953 1215.9268010000233 1219.0364339998923 1219.512301000068 1227.5016089999117 1228.1908070000354 1229.911655999953 1230.3849839998875 1232.1945259999484 1235.2178350000177 1235.858932999894 1239.4475519999396 1239.5959280000534 1241.856322000036 1242.7526879999787 1243.0440229999367 1245.2009330000728 1245.5374119998887 1246.2876709999982 1248.955135999946 1250.5429990000557 1250.582504000049 1251.2098069998901 1251.2951849999372 1254.1131629999727 1255.8238870000932 1257.0559270000085 1259.203206999926 1259.2524230000563 1259.7227469999343 1261.3281219999772 1262.7226120000705 1263.1689450000413 1263.1799900000915 1264.0152970000636 1264.9007699999493 1270.3848949999083 1271.4243469999637 1276.9070429999847 1277.1039539999329 1280.882123999996 1282.9661379999015 1284.8555819999892 1285.2430300000124 1286.5040929999668 1292.4288600001018 1300.4227239999454 1313.656442000065
App start runJsBundle Baseline
Mean: 821.877 ms
Stdev: 30.728 ms (3.7%)
Runs: 769 772 779 779 785 786 787 792 794 797 797 799 800 801 802 802 804 805 805 806 806 807 808 808 809 809 809 812 814 815 818 819 821 821 827 829 829 830 830 831 831 831 840 842 842 843 855 857 857 864 864 876 881 882 883 889 897

Current
Mean: 876.907 ms
Stdev: 24.321 ms (2.8%)
Runs: 822 823 833 838 843 845 846 857 858 860 860 862 862 864 865 865 865 867 869 869 870 870 871 872 872 873 873 875 876 877 878 880 880 882 883 883 883 885 888 889 890 892 896 897 899 902 904 913 914 919 921 923 924 926

Meaningless Changes To Duration

Show entries
Name Duration
Open Search Page TTI 702.057 ms → 732.745 ms (+30.689 ms, +4.4%)
App start regularAppStart 0.014 ms → 0.016 ms (+0.001 ms, +9.1%)
App start nativeLaunch 22.083 ms → 21.263 ms (-0.820 ms, -3.7%)
Show details
Name Duration
Open Search Page TTI Baseline
Mean: 702.057 ms
Stdev: 42.699 ms (6.1%)
Runs: 624.5714930000249 626.0346680001821 628.2204589999747 632.4012450000737 645.5448000000324 650.0458980000112 652.7019460001029 654.0734059999231 655.9361580000259 660.2225349999499 660.6674000001512 661.3965249999892 662.8817959998269 663.3282480000053 664.9254150001798 667.0730389999226 669.7142750001512 670.036174000008 672.3454999998212 672.6729340001475 674.7518720000517 674.8120530000888 676.2219650000334 680.3144129998982 682.2131760001648 682.6201989999972 683.1537279998884 685.4683030000888 689.9956459999084 691.4372560000047 701.1564129998442 703.1973480000161 709.4305419998709 709.8032230001409 712.7603769998532 714.426962000085 718.6472989998292 719.68684999994 720.1081139999442 721.3869630000554 724.3828940000385 733.2591549998615 734.0315759999212 735.9978849999607 736.5259200001601 740.9695230000652 741.1496180000249 741.7708330000751 746.0444340000395 749.679646999808 751.0351149998605 754.4031169998925 755.1649179998785 756.5712080001831 756.8714610000607 758.4139000000432 761.6931969998404 771.9538980000652 772.4136959998868 774.9299319998827 781.8073740000837

Current
Mean: 732.745 ms
Stdev: 48.272 ms (6.6%)
Runs: 633.5901689999737 657.9098720001057 667.0015060000587 668.3422850000206 670.5517579999287 671.0845950001385 671.1893319999799 673.5498860001098 676.3647869999986 676.8395589999855 677.4833169998601 681.921427999856 683.6062020000536 687.0387369999662 688.3536789999343 689.8698740000837 695.2113040001132 702.0792639998253 704.5385340000503 708.946127000032 709.4051930001006 711.2987469998188 714.7082529999316 715.5214440000709 717.9640709999949 718.7593989998568 723.5102949999273 725.5788579999935 726.0135099999607 726.3285330000799 732.411010999931 733.2719320000615 734.9403489998076 735.4041749997996 739.6199550000019 746.5881350000855 747.2826339998282 747.3262130001094 748.1452639999334 749.9038900001906 750.666748000076 753.9436449999921 754.1279710000381 756.2476810000371 756.3659669999033 757.7883299998939 761.0496020000428 764.6516929999925 777.2457280000672 778.2524419999681 785.7904059998691 787.7016199999489 793.0961110000499 802.3476559999399 803.4980060001835 806.4252530001104 808.4051520000212 814.0998949999921 824.768798999954 825.494709999999 846.039184999885
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (5.2%)
Runs: 0.012531999964267015 0.012858000118285418 0.013142999960109591 0.013264999957755208 0.013306000037118793 0.013306000037118793 0.013306000037118793 0.013347000116482377 0.0134680001065135 0.0134680001065135 0.013508999953046441 0.013550000032410026 0.013590999878942966 0.01359100011177361 0.01371199986897409 0.013712000101804733 0.013956000097095966 0.013956000097095966 0.01395700010471046 0.014038000022992492 0.014038000022992492 0.01407799986191094 0.014078999869525433 0.014160000020638108 0.014200999867171049 0.014201000100001693 0.01424099993892014 0.01424099993892014 0.014242000179365277 0.014282000018283725 0.01432300009764731 0.01432300009764731 0.01432300009764731 0.01432300009764731 0.014362999936565757 0.014404000015929341 0.014526000013574958 0.014566999860107899 0.014566999860107899 0.014567000092938542 0.014567000092938542 0.014567000092938542 0.014607999939471483 0.014649000018835068 0.014689000090584159 0.014689000090584159 0.014811000088229775 0.01509599993005395 0.015137000009417534 0.01525900000706315 0.01525900000706315 0.015339999925345182 0.015381000004708767 0.015706000151112676 0.015706999925896525 0.015787999844178557 0.015868999995291233

Current
Mean: 0.016 ms
Stdev: 0.001 ms (7.6%)
Runs: 0.01383400009945035 0.013956999871879816 0.014118999941274524 0.014118999941274524 0.014160000020638108 0.014201000100001693 0.01432300009764731 0.014362999936565757 0.014362999936565757 0.01436399994418025 0.014485999941825867 0.01448600017465651 0.014526000013574958 0.014566999860107899 0.014567000092938542 0.014567000092938542 0.014689000090584159 0.014811999863013625 0.01485200016759336 0.014892000006511807 0.015096000162884593 0.015137000009417534 0.015137000009417534 0.015176999848335981 0.015177000081166625 0.01521800016053021 0.01521800016053021 0.015219000168144703 0.015300000086426735 0.015381000004708767 0.015421000076457858 0.015421000076457858 0.015422000084072351 0.015503000002354383 0.015503000002354383 0.015584999928250909 0.01574800000526011 0.0157880000770092 0.015828000148758292 0.015868999995291233 0.015949999913573265 0.01595000014640391 0.016276000067591667 0.016315999906510115 0.016357999993488193 0.016398000065237284 0.016438999911770225 0.016600999981164932 0.01676399982534349 0.016804999904707074 0.0168869998306036 0.017253000056371093 0.017293000128120184 0.017333999974653125 0.01814799988642335 0.018229000037536025 0.01831099996343255 0.018880000105127692
App start nativeLaunch Baseline
Mean: 22.083 ms
Stdev: 2.704 ms (12.2%)
Runs: 18 18 19 19 19 19 19 19 20 20 20 20 20 20 20 20 20 20 20 21 21 21 21 21 21 21 21 21 21 21 21 21 22 22 22 22 22 22 22 22 23 23 23 23 23 24 24 24 25 25 25 26 26 26 26 27 27 27 29 30

Current
Mean: 21.263 ms
Stdev: 2.156 ms (10.1%)
Runs: 18 18 18 19 19 19 19 19 19 19 20 20 20 20 20 20 20 20 20 20 20 20 20 20 21 21 21 21 21 21 21 21 21 21 21 21 21 21 22 22 22 22 22 22 23 23 23 23 23 24 24 24 26 26 26 27 27

@github-actions
Copy link
Contributor

@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/chiragsalian in version: 1.3.93-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.93-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/chiragsalian in version: 1.3.94-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

1 similar comment
@OSBotify
Copy link
Contributor

OSBotify commented Nov 1, 2023

🚀 Deployed to staging by https://github.com/chiragsalian in version: 1.3.94-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

OSBotify commented Nov 2, 2023

🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.94-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DeployBlockerCash This issue or pull request should block deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants